Remove unused PInvokes #2828
Remove unused PInvokes #2828
Conversation
* Changed the exception type thrown by SQL.SocketDidNotThrow from InternalException. * Excluded InternalException and NetEventSource from compilation in .NET 8.0 target.
JRahnama
added
the
➕ Code Health
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/Interop/SNINativeMethodWrapper.cs
Outdated
Show resolved
Hide resolved
|
Please resolve conflicts to help us move forward with reviews. |
|
Thanks @cheenamalhotra - now resolved, and being CI'd. |
|
I'm working on dropping net6 support here #2927 |
Follows the merge of SqlFileStream.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2828 +/- ##
===========================================
+ Coverage 71.92% 92.58% +20.66%
===========================================
Files 294 6 -288
Lines 60342 310 -60032
===========================================
- Hits 43398 287 -43111
+ Misses 16944 23 -16921
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
In general I'm happy to take this change! However, we've kinda got a three-headed race between you, me, an @mdaigle that'll be challenging to sort out. So here's what I'll propose to make it go more smoothly:
- Let's take this PR
- Let's leave the files in netcore alone for now, though
- I'll get my chain of PRs for moving interop files into the common project done
- These will now delete a lot of it (net6 code), but also address most of the concerns you had for how to move them.
- @edwardneal can you send out a PR after that that removes InternalException and NetEventSource.Common?
- I'm going to continue the merging process with SNI and LocalDBAPI stuff. I selfishly want to do the safe/nativemethods migration, but if I won't stop you if you want to do it :)
|
That sounds sensible to me, thanks. I've noticed that we're working in similar areas! I've reverted the changes in netcore as suggested, but have left
I've got no problem stepping away from the safe/nativemethods migration there. A complete merge of the SNI PInvokes will likely need changes to the native SNI package, so I think that work would have to come to you and the rest of the team whatever happens. I'm interested to see how we merge LocalDBAPI. There were enough differences between netcore and netfx that I'd kicked it into the long grass while I tried to deal with the simple merges. |
|
I think that all sounds good! I just don't want to steal your thunder 😛 We really appreciate your contributions and feedback! The main reason I got into the interop/pinvoke stuff was because I started on LocalDBAPI, then found it had too many dependencies on SNINativeWrapper. So I started merging SNINativeWrapper, and found it had too many dependencies on Interop. So I started merging the Interop stuff... The LocalDBAPI and SNINativeWrapper stuff is going to be challenging to merge since the code isn't aligned the same in both versions (in at least one case, split between files in one version, and in another all in the same file). Either way, let's get one more review on this one 👍 |
This is the first step of a process to merge PInvokes between the .NET and .NET Framework projects. I've started by removing most of the unused PInvokes from both projects. There shouldn't be any surprises in CI.
I've also changed SqlUtil's
SocketDidNotThrowmethod to throw an Exception rather than an InternalException. This was the only reference to InternalException which for the .NET 8.0 target, so this change means that when .NET 6.0 support is dropped, everything in the netcore/src/Common/src/System folder except for NotImplemented.cs can be removed.The next step of the PInvoke merge process will be more involved. What's the coding standard for this? dotnet/runtime uses one folder per OS, one subfolder per DLL, one file per method, and there are quite a few files which fit this pattern. Other parts of the .NET project have a single file containing every PInvoke for a single DLL (the SNI DLL.)
Step 2 will be to merge the OS PInvokes which are needed for .NET Framework and .NET 8.0, and step 3 is currently to address the native SNI PInvokes. At the point of step 3, I'd also like to change the .NET Framework project to refer to Microsoft.Data.SqlClient.SNI.runtime, so it matches the .NET project. This might depend on #2671.